feat(wallet): strict address validation in forest-wallet CLI#6968
Open
0xDevNinja wants to merge 1 commit intoChainSafe:mainfrom
Open
feat(wallet): strict address validation in forest-wallet CLI#69680xDevNinja wants to merge 1 commit intoChainSafe:mainfrom
0xDevNinja wants to merge 1 commit intoChainSafe:mainfrom
Conversation
Mirrors the `forest-cli` treatment introduced in ChainSafe#6011 for the `forest-wallet` binary. Address arguments to `balance`, `export`, `has`, `delete`, `set-default`, `sign`, `verify` and the `--from` option of `send` are now parsed into `StrictAddress` at CLI-parse time. clap surfaces a `ValueValidation` error on a malformed input instead of deferring to a runtime `StrictAddress::from_str(...)?` fallback, and each handler simply destructures the typed value via `field: StrictAddress(inner)`. Network detection is moved ahead of `Cli::parse_from` so that clap-time `StrictAddress` validation accepts testnet (`t0...`) addresses when the daemon reports a testnet chain. Client construction errors propagate (matching `forest-cli`); an unreachable daemon leaves the global `CurrentNetwork` at its mainnet default. `wallet_default_address()` now returns `Option<Address>` rather than `Option<String>`, removing a stringify-then-reparse round trip through `StrictAddress::from_str` in both the `list` and `send` handlers. `Send.target_address` is intentionally kept as a `String` because it accepts ETH (`0x...`) recipients, which `StrictAddress` rejects; `resolve_target_address` continues to handle the dispatch. `ValidateAddress.address` also stays a `String`, since its purpose is to validate arbitrary user input. Two clap-level regression tests guard against an accidental revert of the `StrictAddress` typing. Refs ChainSafe#6012
0xDevNinja
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Contributor
WalkthroughThis PR implements strict address validation in the forest-wallet CLI by shifting address parsing from runtime to compile-time. It uses typed addresses (StrictAddress) for command parameters, adds early network detection before CLI parsing to set the correct network context, and includes tests verifying malformed addresses are rejected during parsing. Changes
Sequence DiagramsequenceDiagram
participant Main as main.rs
participant RPC as rpc::Client
participant GlobalNet as GlobalNetwork
participant CLIParser as CLI Parser
participant Handler as Command Handler
rect rgba(100, 150, 200, 0.5)
Note over Main,GlobalNet: NEW: Early Network Detection (Before Parsing)
Main->>RPC: Initialize client (no token)
RPC-->>Main: Connected to daemon
Main->>RPC: Detect daemon's network chain
RPC-->>Main: Return NetworkChain
Main->>GlobalNet: Set CurrentNetwork
end
rect rgba(150, 100, 200, 0.5)
Note over CLIParser,Handler: CLI Parsing with Address Validation
Main->>CLIParser: Parse CLI args with CurrentNetwork set
CLIParser->>CLIParser: Validate StrictAddress fields<br/>(network prefix checks)
alt Address Valid
CLIParser-->>Main: Return parsed command
Main->>Handler: Execute command with typed addresses
else Address Invalid
CLIParser-->>Main: Return validation error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/wallet/main.rs (1)
24-28: Add a debug/warn log when the network probe fails and mainnet fallback is used.On Line 24, probe failures are silently ignored; a small log here would make prefix-validation fallbacks much easier to diagnose.
Suggested adjustment
- if let Ok(name) = StateNetworkName::call(&client, ()).await - && !matches!(NetworkChain::from_str(&name), Ok(NetworkChain::Mainnet)) - { - CurrentNetwork::set_global(Network::Testnet); - } + match StateNetworkName::call(&client, ()).await { + Ok(name) if !matches!(NetworkChain::from_str(&name), Ok(NetworkChain::Mainnet)) => { + CurrentNetwork::set_global(Network::Testnet); + } + Ok(_) => {} + Err(err) => { + tracing::debug!( + %err, + "network probe failed; keeping CurrentNetwork at mainnet default for CLI parsing" + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/main.rs` around lines 24 - 28, The probe using StateNetworkName::call(&client, ()).await currently swallows failures; update the branch around StateNetworkName::call, NetworkChain::from_str and the fallback that calls CurrentNetwork::set_global(Network::Testnet) to log a warning or debug message when the call fails or when parsing does not yield Mainnet. Specifically, capture the Err from StateNetworkName::call and log the error (including the error object and context like "network probe failed, falling back to Testnet"), and also log when NetworkChain::from_str returns non-Mainnet to explain why CurrentNetwork::set_global(Network::Testnet) is invoked.src/wallet/subcommands/mod.rs (1)
48-69: Add a clap regression test forsend --frommalformed addresses.
balanceandsignare covered, butsend --fromwas also migrated toStrictAddress. A matchingValueValidationtest would prevent regressions in that path.Suggested test addition
#[test] fn wallet_sign_rejects_malformed_address() { assert_eq!( parse_err_kind(&[ "forest-wallet", "sign", "-m", "deadbeef", "-a", "not-an-address", ]), clap::error::ErrorKind::ValueValidation, ); } + + #[test] + fn wallet_send_from_rejects_malformed_address() { + assert_eq!( + parse_err_kind(&[ + "forest-wallet", + "send", + "--from", + "not-an-address", + "f01234", + "1", + ]), + clap::error::ErrorKind::ValueValidation, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/mod.rs` around lines 48 - 69, Add a new unit test similar to wallet_balance_rejects_malformed_address and wallet_sign_rejects_malformed_address that asserts parse_err_kind returns clap::error::ErrorKind::ValueValidation for a malformed address passed to the send command's --from flag; use the existing helper parse_err_kind and name the test wallet_send_from_rejects_malformed_address, and supply the minimal extra send arguments (e.g. to and amount) so the clap parser reaches --from validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wallet/main.rs`:
- Around line 24-28: The probe using StateNetworkName::call(&client, ()).await
currently swallows failures; update the branch around StateNetworkName::call,
NetworkChain::from_str and the fallback that calls
CurrentNetwork::set_global(Network::Testnet) to log a warning or debug message
when the call fails or when parsing does not yield Mainnet. Specifically,
capture the Err from StateNetworkName::call and log the error (including the
error object and context like "network probe failed, falling back to Testnet"),
and also log when NetworkChain::from_str returns non-Mainnet to explain why
CurrentNetwork::set_global(Network::Testnet) is invoked.
In `@src/wallet/subcommands/mod.rs`:
- Around line 48-69: Add a new unit test similar to
wallet_balance_rejects_malformed_address and
wallet_sign_rejects_malformed_address that asserts parse_err_kind returns
clap::error::ErrorKind::ValueValidation for a malformed address passed to the
send command's --from flag; use the existing helper parse_err_kind and name the
test wallet_send_from_rejects_malformed_address, and supply the minimal extra
send arguments (e.g. to and amount) so the clap parser reaches --from
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f00574d-3b2b-42b7-be1a-1f0963ceca9c
📒 Files selected for processing (4)
CHANGELOG.mdsrc/wallet/main.rssrc/wallet/subcommands/mod.rssrc/wallet/subcommands/wallet_cmd.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
forest-walletsubcommands toStrictAddress(parsed at clap time). Coversbalance,export,has,delete,set-default,sign,verify, and the--fromoption ofsend. Handlers destructure the typed value viafield: StrictAddress(inner); runtimeStrictAddress::from_str(...)?fallbacks are removed.Cli::parse_frominsrc/wallet/main.rsso that clap-timeStrictAddressvalidation accepts testnet (t0...) addresses when the daemon reports a testnet chain. Client construction errors propagate (mirroringforest-cliafter feat: strict checks for address args inforest-cli#6011); an unreachable daemon leaves the globalCurrentNetworkat its mainnet default.wallet_default_address()return type toOption<Address>(wasOption<String>), removing a stringify→reparse round trip throughStrictAddress::from_strin both thelistandsendhandlers.balance, one forsign -a ...) assertingValueValidationon malformed addresses, guarding against an accidental revert.CHANGELOG.md:Addedentry listing the migrated subcommands and explicitly calling out thesend target_addressnon-migration.Intentional non-migrations
sendpositionaltarget_addressstaysString:resolve_target_addressaccepts both FIL (f0.../t0...) and ETH (0x...) forms, butStrictAddress::FromStrrejects the ETH form, so typing it would break the existing ETH-recipient path. Deviates from the issue body, which assumed all address fields could beStrictAddress.validate-addresspositionaladdressstaysString: the purpose of the subcommand is to validate arbitrary user input.Reference issue to close (if applicable)
Closes #6012
Other information and links
forest-cli#6011 (same change forforest-cli, including the pre-parse network detection).Recipientenum (Fil(StrictAddress),Eth(EthAddress)) forsend target_address, so the ETH form gets the same compile-time check.Change checklist
Outside contributions
AI Usage Disclosure
This PR was prepared with assistance from Claude Code (Anthropic). Extent:
forest-cli#6011), implementation, two self-review passes with an independent reviewer agent, test design, and PR drafting were AI-assisted.cargo fmt --all -- --check,cargo clippy --profile quick --all-targets --no-deps -- -D warnings(withFOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT=1), andcargo test --lib -- wallet::subcommands(12/12 pass) are all green.Summary by CodeRabbit
Release Notes
New Features
Tests